Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add stdin option to command chains #198

Merged
merged 1 commit into from
May 5, 2024
Merged

Add stdin option to command chains #198

merged 1 commit into from
May 5, 2024

Conversation

maksimowiczm
Copy link
Contributor

@maksimowiczm maksimowiczm commented May 1, 2024 •
edited
Loading

Description

Describe the change. If there is an associated issue, please include the issue link (e.g. "Closes #xxx"). For UI changes, please also include screenshots.

Added stdin field on the !command variant. Stdin is now piped into command using ChildStdin. It is still compatible with previous command variant.

Closes #190

Known Risks

What issues could potentially go wrong with this change? Is it a breaking change? What have you done to mitigate any potential risks?

Calling command may be unsafe, but I don't think it carries any new threat.

QA

How did you test this?

Added one unit test. Probably need to add more.

Checklist

  • Have you read CONTRIBUTING.md already?
  • Did you update CHANGELOG.md?
    • Only user-facing changes belong in the changelog. Internal changes such as refactors should only be included if they'll impact users, e.g. via performance improvement.
  • Did you remove all TODOs?
    • If there are unresolved issues, please open a follow-on issue and link to it in a comment so future work can be tracked

@maksimowiczm
Copy link
Contributor Author

There is still some work to do (docs and tests). I wanted to see what are your thought on this.

LucasPickering
LucasPickering requested changes May 1, 2024
View reviewed changes
Copy link
Owner

@LucasPickering LucasPickering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I tested this locally and it's working great. Just left some feedback on minor tweaks. In addition to adding the field in chain_source.md, could you do a search for /sh.*-c/ in the docs (I found two results) and change those examples to use this new field? Thanks!

src/template/render.rs Outdated Show resolved Hide resolved
src/template/render.rs Outdated Show resolved Hide resolved
src/template/render.rs Outdated Show resolved Hide resolved
src/template/render.rs Outdated Show resolved Hide resolved
src/template/render.rs Outdated Show resolved Hide resolved
src/template.rs Show resolved Hide resolved
@maksimowiczm maksimowiczm force-pushed the master branch 2 times, most recently from a5c3323 to cc61658 Compare May 2, 2024 19:45
@maksimowiczm
Copy link
Contributor Author

Sorry, had to do some force pushes because my editor reformatted md files 😅

@maksimowiczm
Copy link
Contributor Author

I have no idea for any new tests. I think it is ready to merge.

LucasPickering
LucasPickering requested changes May 3, 2024
View reviewed changes
src/template/render.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/template.rs Show resolved Hide resolved
@maksimowiczm
Copy link
Contributor Author

Resolved conversations. Not sure about the test name.

LucasPickering
LucasPickering requested changes May 3, 2024
View reviewed changes
src/template.rs Outdated Show resolved Hide resolved
LucasPickering
LucasPickering approved these changes May 5, 2024
View reviewed changes
Copy link
Owner

@LucasPickering LucasPickering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, you have conflicts though so I can't merge it. Could you rebase onto master please?

@LucasPickering LucasPickering enabled auto-merge (rebase) May 5, 2024 13:57
@LucasPickering
Copy link
Owner

Looks good, you have conflicts though so I can't merge it. Could you rebase onto master please?

Never mind, I forgot I can also make edits. The conflicts where just on the changelog so I resolved and squashed.

@LucasPickering LucasPickering merged commit 65be7ad into LucasPickering:master May 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucasPickering LucasPickering LucasPickering approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add stdin option to command chains
2 participants
@maksimowiczm @LucasPickering